fix(pg): do not treat Sync as connection ending - #3772
Conversation
Connection.sync() was setting _ending=true on every extended-query Sync. That flag is meant for disconnect (Terminate / end()), so after the first parameterized query ECONNRESET and EPIPE were swallowed for the life of the connection. Keep _ending only on end() and connect-timeout teardown. Fixes brianc#3769
brianc
left a comment
brianc
left a comment
There was a problem hiding this comment.
Oh yeah I see this fix makes sense. What I want to see tho is an integration tests w/ an actual postgres backend to trigger the error path if possible. Unit tests are a pretty weak gaurentee of correctness compared to integration tests which actually test "This code works now against a backend and it didn't use to before the patch." Would you be able to include an integration test? (under packages/pg/test/integration/gh-issues/3772-tests.js if possible)
Adds a real-backend integration test under test/integration/gh-issues/ covering the bug from brianc#3769: Connection.prototype.sync() used to set _ending = true, so every healthy extended-protocol connection was left looking like it was ending. reportStreamError drops ECONNRESET/EPIPE while _ending is set, so a genuine mid-query teardown was silently swallowed and only the generic close-path error surfaced. The tests use a real PostgreSQL backend reached through a local TCP proxy, so the connection reset can be triggered deterministically: 1. a real extended-protocol query does not mark the connection as ending (fails before the fix) 2. a mid-query connection reset is reported, not swallowed by Sync (fails before the fix: no ECONNRESET reaches the client) Both tests fail on the pre-patch code and pass with the fix.
|
Added the integration test you asked for at It runs both cases against a real Postgres backend:
I verified the "it didn't use to" part by swapping only
Before the fix, case 2 only surfaces the generic Local runs:
Two other integration files fail on my machine for environment reasons, not from this change: Happy to rework the teardown approach if you'd prefer it driven a different way. |
The integration suite runs twice: once with the JS implementation and once with a `native` argument that swaps in the libpq bindings. The native client has no `connection` (and therefore no `_ending`), so the state-machine assertions threw a TypeError and aborted the run via the helper's uncaughtException handler. Guard on helper.args.native, matching the existing idiom in test/integration/client/pipeline-portal-tests.js.
|
One follow-up on my last comment: the first push of the integration test turned CI red, and I've fixed it. The integration suite runs twice, once against the JS implementation and once with a CI is now green across all 12 checks (Node 16 through 26, PostgreSQL 13 through 18, plus lint). |
brianc
left a comment
brianc
left a comment
There was a problem hiding this comment.
nice! with the integration test I feel 💯 about merging this, thanks. I think its probably time I write an AGENTS.md in the repo to give instructions to folks who are using agents to make sure to include integration tests when possible as unit tests are generally far less valuable.
|
thank you for putting in the work here & the follow up! Appreciated! |
|
Thank you for merging, Brian! This PR turned out to be a real learning experience with the extended query protocol. I went in thinking Sync was just a message boundary, and came out with a proper understanding of why it exists as the barrier after Parse/Bind/Execute, and how The Thanks as well for the quick review, and for maintaining a codebase that made tracing this straightforward. Happy to help with anything else that comes up in the repo. |
2 participants
Summary
Fixes #3769.
Connection.sync()was setting_ending = trueon every extended-query Sync. That flag exists soreportStreamErrorcan ignoreECONNRESET/EPIPEduring disconnect. Sync is the protocol barrier after Parse/Bind/Execute, not a disconnect, so after the first parameterized query those socket errors were silently dropped for the rest of the connection lifetime.With
pipeline: truethat interacts badly with unexpected pooler/socket teardowns: the normal error path is closed and recovery depends only on the asyncclose/endpath, which can leave an in-flight query promise unsettled.Changes
_endinginsync(); leave it set only inend()(Terminate) and the connect-timeout teardown path that already setscon._ending = truebefore destroying the stream._endingfalse and thatECONNRESETafter Sync still emitserror(existing disconnect coverage still usesend()).Test plan
node test/unit/connection/error-tests.js(new cases green)packages/pgunit suite (find test/unit -name '*-tests.js' | xargs -n1 node) - 284 pass